Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update AMP validator spec to 2011140244000 #5608

Merged
merged 7 commits into from
Nov 20, 2020

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Nov 17, 2020

Previously #5356.

  • Run ./bin/amphtml-update.sh (lando ssh -c 'bash ./bin/amphtml-update.sh vendor/amphtml').
  • Examine diff for changelog.
  • Examine upstream diff to ensure nothing was missed.
  • Update spec generator as needed based on spec format changes.
  • Modify validating sanitizer based on changes to spec, if needed.
  • Add tests for key changes.

Changelog

Stories

  • Allow span in addition to a as child of amp-story-player.
  • Add attributes to amp-story-interactive-results: chip-style and option-[1-4]-results-threshold. Nevertheless, there is a bug in the value_regex. See 🐛 Fix value_regex for decimal option-*-results-threshold attr values amphtml#31241.
  • Update amp-story-360:
    • Add attributes: controls, scene-heading, scene-pitch, and scene-roll.
    • Remove responsive layout.
    • Allow amp-video child in addition to amp-img.
  • Replace 1.0 with 0.1 for amp-story-interactive component versions.

Details

(
    PREV_VERSION=2008290323001;
    THIS_VERSION=2011140244000;
    git checkout $THIS_VERSION;
    git diff $PREV_VERSION...$THIS_VERSION -w -- $( git ls-files | grep '.protoascii' );
    git checkout - > /dev/null
)
2008290323001...2011140244000
diff --git a/extensions/amp-ad-custom/validator-amp-ad-custom.protoascii b/extensions/amp-ad-custom/validator-amp-ad-custom.protoascii
index 9009cf4e8..c6e5528bc 100644
--- a/extensions/amp-ad-custom/validator-amp-ad-custom.protoascii
+++ b/extensions/amp-ad-custom/validator-amp-ad-custom.protoascii
@@ -16,7 +16,6 @@
 tags: {  # amp-ad-custom
   html_format: AMP  # AMP only as ads are not allowed inside AMP Ads
   tag_name: "SCRIPT"
-  spec_name: "amp-ad-custom extension .js script"
   extension_spec: {
     name: "amp-ad-custom"
     version: "0.1"
diff --git a/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii b/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
index 4f4cbdcfb..55b28076d 100644
--- a/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
+++ b/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
@@ -65,6 +65,7 @@ tags: {  # <amp-autocomplete>
   attrs: { name: "max-entries" }
   attrs: { name: "max-items" }
   attrs: { name: "min-characters" }
+  attrs: { name: "prefetch" }
   attrs: {
     name: "query"
     trigger: {
@@ -107,6 +108,7 @@ tags: {  # <amp-autocomplete>
   attrs: { name: "items" }
   attrs: { name: "max-items" }
   attrs: { name: "min-characters" }
+  attrs: { name: "prefetch" }
   attrs: {
     name: "query"
     trigger: {
diff --git a/extensions/amp-story-player/validator-amp-story-player.protoascii b/extensions/amp-onetap-google/validator-amp-onetap-google.protoascii
similarity index 65%
copy from extensions/amp-story-player/validator-amp-story-player.protoascii
copy to extensions/amp-onetap-google/validator-amp-onetap-google.protoascii
index a798d42c3..fb12e8960 100644
--- a/extensions/amp-story-player/validator-amp-story-player.protoascii
+++ b/extensions/amp-onetap-google/validator-amp-onetap-google.protoascii
@@ -14,28 +14,32 @@
 # limitations under the license.
 #
 
-tags: {  # amp-story-player
+tags: {  # amp-onetap-google
   html_format: AMP
   tag_name: "SCRIPT"
   extension_spec: {
-    name: "amp-story-player"
+    name: "amp-onetap-google"
     version: "0.1"
     version: "latest"
   }
   attr_lists: "common-extension-attrs"
 }
-tags: {  # <amp-story-player>
+
+tags: {  # <amp-onetap-google>
   html_format: AMP
-  tag_name: "AMP-STORY-PLAYER"
-  requires_extension: "amp-story-player"
-  child_tags: {
-    child_tag_name_oneof: "A"
+  tag_name: "AMP-ONETAP-GOOGLE"
+  requires_extension: "amp-onetap-google"
+  unique: true
+  # data-* is generally allowed, but it's not generally mandatory.
+  attrs: {
+    name: "data-src"
+    mandatory: true,
+    value_url: {
+      protocol: "https"
+    }
   }
+  attr_lists: "extended-amp-global"
   amp_layout: {
-    supported_layouts: FILL
-    supported_layouts: FIXED
-    supported_layouts: FIXED_HEIGHT
-    supported_layouts: FLEX_ITEM
-    supported_layouts: RESPONSIVE
+    supported_layouts: NODISPLAY
   }
 }
diff --git a/extensions/amp-story-360/validator-amp-story-360.protoascii b/extensions/amp-story-360/validator-amp-story-360.protoascii
index 3812325a3..933941a70 100644
--- a/extensions/amp-story-360/validator-amp-story-360.protoascii
+++ b/extensions/amp-story-360/validator-amp-story-360.protoascii
@@ -30,37 +30,63 @@ tags: {  # <amp-story-360>
   requires_extension: "amp-story-360"
   mandatory_ancestor: "AMP-STORY"
   spec_url: "https://amp.dev/documentation/components/amp-story-360"
+  attrs: {
+    name: "controls"
+    value: "gyroscope"
+  }
   attrs: {
     name: "duration"
     value_regex: "([0-9\\.]+)\\s*(s|ms)"
   }
+  attrs: {
+    name: "heading-end"
+    value_regex: "-?\\d+\\.?\\d*"
+    trigger: {
+      also_requires_attr: "duration"
+    }
+  }
   attrs: {
     name: "heading-start"
     value_regex: "-?\\d+\\.?\\d*"
   }
+  attrs: {
+    name: "pitch-end"
+    value_regex: "-?\\d+\\.?\\d*"
+    trigger: {
+      also_requires_attr: "duration"
+    }
+  }
   attrs: {
     name: "pitch-start"
     value_regex: "-?\\d+\\.?\\d*"
   }
   attrs: {
-    name: "zoom-start"
-    value_regex: "\\d+\\.?\\d*"
+    name: "scene-heading"
+    value_regex: "-?\\d+\\.?\\d*"
   }
   attrs: {
-    name: "heading-end"
+    name: "scene-pitch"
     value_regex: "-?\\d+\\.?\\d*"
   }
   attrs: {
-    name: "pitch-end"
+    name: "scene-roll"
     value_regex: "-?\\d+\\.?\\d*"
   }
   attrs: {
     name: "zoom-end"
     value_regex: "\\d+\\.?\\d*"
+    trigger: {
+      also_requires_attr: "duration"
+    }
+  }
+  attrs: {
+    name: "zoom-start"
+    value_regex: "\\d+\\.?\\d*"
   }
   child_tags: {
     mandatory_num_child_tags: 1
     child_tag_name_oneof: "AMP-IMG"
+    child_tag_name_oneof: "AMP-VIDEO"
   }
   amp_layout: {
     supported_layouts: FILL
@@ -68,6 +94,5 @@ tags: {  # <amp-story-360>
     supported_layouts: FIXED_HEIGHT
     supported_layouts: FLEX_ITEM
     supported_layouts: NODISPLAY
-    supported_layouts: RESPONSIVE
   }
 }
diff --git a/extensions/amp-story-interactive/validator-amp-story-interactive.protoascii b/extensions/amp-story-interactive/validator-amp-story-interactive.protoascii
index 7a5eab5e6..132cad177 100644
--- a/extensions/amp-story-interactive/validator-amp-story-interactive.protoascii
+++ b/extensions/amp-story-interactive/validator-amp-story-interactive.protoascii
@@ -19,7 +19,7 @@ tags: {  # amp-story-interactive
   tag_name: "SCRIPT"
   extension_spec: {
     name: "amp-story-interactive"
-    version: "1.0"
+    version: "0.1"
     version: "latest"
   }
   attr_lists: "common-extension-attrs"
@@ -29,24 +29,30 @@ tags: {  # <amp-story-interactive-quiz>
   tag_name: "AMP-STORY-INTERACTIVE-QUIZ"
   requires_extension: "amp-story-interactive"
   mandatory_ancestor: "AMP-STORY-GRID-LAYER"
-  attr_lists: 'interactive-options-text-attrs'
-  attr_lists: 'interactive-options-confetti-attrs'
-  attr_lists: 'interactive-shared-configs-attrs'
+  attr_lists: "interactive-options-text-attrs"
+  attr_lists: "interactive-options-confetti-attrs"
+  attr_lists: "interactive-shared-configs-attrs"
   attrs: {
-    name: 'option-1-correct'
+    name: "option-1-correct"
     mandatory_oneof: "['option-1-correct', 'option-2-correct', 'option-3-correct', 'option-4-correct']"
   }
   attrs: {
-    name: 'option-2-correct'
+    name: "option-2-correct"
     mandatory_oneof: "['option-1-correct', 'option-2-correct', 'option-3-correct', 'option-4-correct']"
   }
   attrs: {
-    name: 'option-3-correct'
+    name: "option-3-correct"
     mandatory_oneof: "['option-1-correct', 'option-2-correct', 'option-3-correct', 'option-4-correct']"
+    trigger: {
+      also_requires_attr: "option-3-text"
+    }
   }
   attrs: {
-    name: 'option-4-correct'
+    name: "option-4-correct"
     mandatory_oneof: "['option-1-correct', 'option-2-correct', 'option-3-correct', 'option-4-correct']"
+    trigger: {
+      also_requires_attr: "option-4-text"
+    }
   }
 }
 tags: {  # <amp-story-interactive-poll>
@@ -54,30 +60,30 @@ tags: {  # <amp-story-interactive-poll>
   tag_name: "AMP-STORY-INTERACTIVE-POLL"
   requires_extension: "amp-story-interactive"
   mandatory_ancestor: "AMP-STORY-GRID-LAYER"
-  attr_lists: 'interactive-options-text-attrs'
-  attr_lists: 'interactive-options-confetti-attrs'
-  attr_lists: 'interactive-options-results-category-attrs'
-  attr_lists: 'interactive-shared-configs-attrs'
+  attr_lists: "interactive-options-text-attrs"
+  attr_lists: "interactive-options-confetti-attrs"
+  attr_lists: "interactive-options-results-category-attrs"
+  attr_lists: "interactive-shared-configs-attrs"
 }
 tags: {  # <amp-story-interactive-binary-poll>
   html_format: AMP
   tag_name: "AMP-STORY-INTERACTIVE-BINARY-POLL"
   requires_extension: "amp-story-interactive"
   mandatory_ancestor: "AMP-STORY-GRID-LAYER"
-  attr_lists: 'interactive-shared-configs-attrs'
+  attr_lists: "interactive-shared-configs-attrs"
   attrs: {
-    name: 'option-1-text'
+    name: "option-1-text"
     mandatory: true
   }
   attrs: {
-    name: 'option-2-text'
+    name: "option-2-text"
     mandatory: true
   }
   attrs: {
-    name: 'option-1-confetti'
+    name: "option-1-confetti"
   }
   attrs: {
-    name: 'option-2-confetti'
+    name: "option-2-confetti"
   }
 }
 tags: {  # <amp-story-interactive-results>
@@ -86,133 +92,192 @@ tags: {  # <amp-story-interactive-results>
   requires_extension: "amp-story-interactive"
   mandatory_ancestor: "AMP-STORY-GRID-LAYER"
   attrs: {
-    name: 'option-1-results-category'
+    name: "option-1-results-category"
     mandatory: true
   }
   attrs: {
-    name: 'option-2-results-category'
+    name: "option-2-results-category"
     mandatory: true
   }
   attrs: {
-    name: 'option-3-results-category'
+    name: "option-3-results-category"
+  }
+  attrs: {
+    name: "option-4-results-category"
+    trigger: {
+      also_requires_attr: "option-3-results-category"
+    }
+  }
+  attrs: {
+    name: "option-1-image"
+  }
+  attrs: {
+    name: "option-2-image"
+  }
+  attrs: {
+    name: "option-3-image"
+    trigger: {
+      also_requires_attr: "option-3-results-category"
+    }
+  }
+  attrs: {
+    name: "option-4-image"
+    trigger: {
+      also_requires_attr: "option-4-results-category"
+    }
   }
   attrs: {
-    name: 'option-4-results-category'
+    name: "option-1-text"
   }
   attrs: {
-    name: 'option-1-image'
+    name: "option-2-text"
   }
   attrs: {
-    name: 'option-2-image'
+    name: "option-3-text"
+    trigger: {
+      also_requires_attr: "option-3-results-category"
+    }
   }
   attrs: {
-    name: 'option-3-image'
+    name: "option-4-text"
+    trigger: {
+      also_requires_attr: "option-4-results-category"
+    }
   }
   attrs: {
-    name: 'option-4-image'
+    name: "option-1-results-threshold"
+    value_regex: "\\d+[.\\d+]?"
   }
   attrs: {
-    name: 'option-1-text'
+    name: "option-2-results-threshold"
+    value_regex: "\\d+[.\\d+]?"
   }
   attrs: {
-    name: 'option-2-text'
+    name: "option-3-results-threshold"
+    value_regex: "\\d+[.\\d+]?"
+    trigger: {
+      also_requires_attr: "option-3-results-category"
+    }
   }
   attrs: {
-    name: 'option-3-text'
+    name: "option-4-results-threshold"
+    value_regex: "\\d+[.\\d+]?"
+    trigger: {
+      also_requires_attr: "option-4-results-category"
+    }
   }
   attrs: {
-    name: 'option-4-text'
+    name: "prompt-text"
   }
   attrs: {
-    name: 'prompt-text'
+    name: "theme"
+    value: "light"
+    value: "dark"
   }
   attrs: {
-    name: 'theme'
-    value: 'light'
-    value: 'dark'
+    name: "chip-style"
+    value: "flat"
+    value: "transparent"
   }
 }
 
 attr_lists: {
-  name: 'interactive-shared-configs-attrs'
+  name: "interactive-shared-configs-attrs"
   attrs: {
-    name: 'id'
+    name: "id"
     mandatory: true
   }
   attrs: {
-    name: 'prompt-text'
+    name: "prompt-text"
   }
   attrs: {
-    name: 'endpoint'
+    name: "endpoint"
     mandatory: true
     value_url: {
-      protocol: 'https'
+      protocol: "https"
       allow_relative: false
       allow_empty: false
     }
   }
   attrs: {
-    name: 'theme'
-    value: 'light'
-    value: 'dark'
+    name: "theme"
+    value: "light"
+    value: "dark"
   }
   attrs: {
-    name: 'chip-style'
-    value: 'shadow'
-    value: 'flat'
-    value: 'transparent'
+    name: "chip-style"
+    value: "shadow"
+    value: "flat"
+    value: "transparent"
   }
   attrs: {
-    name: 'prompt-size'
-    value: 'small'
-    value: 'medium'
-    value: 'large'
+    name: "prompt-size"
+    value: "small"
+    value: "medium"
+    value: "large"
   }
 }
 attr_lists: {
-  name: 'interactive-options-text-attrs'
+  name: "interactive-options-text-attrs"
   attrs: {
-    name: 'option-1-text'
+    name: "option-1-text"
     mandatory: true
   }
   attrs: {
-    name: 'option-2-text'
+    name: "option-2-text"
     mandatory: true
   }
   attrs: {
-    name: 'option-3-text'
+    name: "option-3-text"
   }
   attrs: {
-    name: 'option-4-text'
+    name: "option-4-text"
+    trigger: {
+      also_requires_attr: "option-3-text"
+    }
   }
 }
 attr_lists: {
-  name: 'interactive-options-confetti-attrs'
+  name: "interactive-options-confetti-attrs"
   attrs: {
-    name: 'option-1-confetti'
+    name: "option-1-confetti"
   }
   attrs: {
-    name: 'option-2-confetti'
+    name: "option-2-confetti"
   }
   attrs: {
-    name: 'option-3-confetti'
+    name: "option-3-confetti"
   }
   attrs: {
-    name: 'option-4-confetti'
+    name: "option-4-confetti"
   }
 }
 attr_lists: {
-  name: 'interactive-options-results-category-attrs'
+  name: "interactive-options-results-category-attrs"
   attrs: {
-    name: 'option-1-results-category'
+    name: "option-1-results-category"
+    trigger: {
+      also_requires_attr: "option-2-results-category"
+    }
   }
   attrs: {
-    name: 'option-2-results-category'
+    name: "option-2-results-category"
+    trigger: {
+      also_requires_attr: "option-1-results-category"
+    }
   }
   attrs: {
-    name: 'option-3-results-category'
+    name: "option-3-results-category"
+    trigger: {
+      also_requires_attr: "option-2-results-category"
+      also_requires_attr: "option-3-text"
+    }
   }
   attrs: {
-    name: 'option-4-results-category'
+    name: "option-4-results-category"
+    trigger: {
+      also_requires_attr: "option-3-results-category"
+      also_requires_attr: "option-4-text"
+    }
   }
 }
diff --git a/extensions/amp-story-player/validator-amp-story-player.protoascii b/extensions/amp-story-player/validator-amp-story-player.protoascii
index a798d42c3..9f714de55 100644
--- a/extensions/amp-story-player/validator-amp-story-player.protoascii
+++ b/extensions/amp-story-player/validator-amp-story-player.protoascii
@@ -28,9 +28,7 @@ tags: {  # <amp-story-player>
   html_format: AMP
   tag_name: "AMP-STORY-PLAYER"
   requires_extension: "amp-story-player"
-  child_tags: {
-    child_tag_name_oneof: "A"
-  }
+  descendant_tag_list: "amp-story-player-allowed-descendants"
   amp_layout: {
     supported_layouts: FILL
     supported_layouts: FIXED
@@ -39,3 +37,9 @@ tags: {  # <amp-story-player>
     supported_layouts: RESPONSIVE
   }
 }
+descendant_tag_list {
+  name: "amp-story-player-allowed-descendants"
+  tag: "A"
+  tag: "SPAN"
+  tag: "I-AMPHTML-SIZER"  # Only allowed when document is transformed AMP
+}
diff --git a/extensions/amp-video-docking/validator-amp-video-docking.protoascii b/extensions/amp-video-docking/validator-amp-video-docking.protoascii
index 2875dfbd7..bfb2c4ad7 100644
--- a/extensions/amp-video-docking/validator-amp-video-docking.protoascii
+++ b/extensions/amp-video-docking/validator-amp-video-docking.protoascii
@@ -17,7 +17,6 @@
 tags: {  # amp-video-docking
   html_format: AMP
   tag_name: "SCRIPT"
-  spec_name: "amp-video-docking"
   extension_spec: {
     name: "amp-video-docking"
     version: "0.1"
diff --git a/extensions/amp-video/validator-amp-video.protoascii b/extensions/amp-video/validator-amp-video.protoascii
index 2b0154991..fba174053 100644
--- a/extensions/amp-video/validator-amp-video.protoascii
+++ b/extensions/amp-video/validator-amp-video.protoascii
@@ -125,6 +125,7 @@ tags: {  # <amp-video> not in amp-story.
     supported_layouts: FIXED
     supported_layouts: FIXED_HEIGHT
     supported_layouts: FLEX_ITEM
+    supported_layouts: INTRINSIC
     supported_layouts: NODISPLAY
     supported_layouts: RESPONSIVE
   }
@@ -147,6 +148,7 @@ tags: {  # <amp-video> in amp-story-page-attachment (same rules as regular AMPHT
     supported_layouts: FIXED
     supported_layouts: FIXED_HEIGHT
     supported_layouts: FLEX_ITEM
+    supported_layouts: INTRINSIC
     supported_layouts: NODISPLAY
     supported_layouts: RESPONSIVE
   }
diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii
index aa6d96612..14181ff3e 100644
--- a/validator/validator-main.protoascii
+++ b/validator/validator-main.protoascii
@@ -26,7 +26,7 @@ min_validator_revision_required: 475
 # newer versions of the spec file. This is currently a Google internal
 # mechanism, validator.js does not use this facility. However, any
 # change to this file (validator-main.js) requires updating this revision id.
-spec_file_revision: 1095
+spec_file_revision: 1119
 
 styles_spec_url: "https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/style_pages/"
 script_spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/validation-workflow/validation_errors/#custom-javascript-is-not-allowed"
@@ -43,7 +43,7 @@ script_spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/valid
 
 doc: {
   html_format: AMP4EMAIL
-  max_bytes: 100000
+  max_bytes: 200000
   max_bytes_spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/email-spec/amp-email-format/?format=email"
 }
 
@@ -1709,9 +1709,10 @@ tags: {
 tags: {
   html_format: AMP  # Disallowed in AMP4ADS because <noscript> is disallowed.
   tag_name: "IMG"
+  spec_name: "noscript > img"
+  descriptive_name: "img"
   mandatory_ancestor: "NOSCRIPT"
   mandatory_ancestor_suggested_alternative: "AMP-IMG"
-  spec_url: "https://amp.dev/documentation/components/amp-img/"
   attrs: { name: "alt" }
   attrs: { name: "attribution" }
   attrs: { name: "border" }  # Not valid html5 but commonly used and supported.
@@ -1722,32 +1723,22 @@ tags: {
     value: "sync"
   }
   attrs: { name: "height" }  # Not valid html5 but commonly used and supported.
-  attrs: { name: "ismap" }
-  attrs: {
-    name: "longdesc"
-    value_url: {
-      protocol: "http"
-      protocol: "https"
-    }
-    disallowed_value_regex: "__amp_source_origin"
-  }
   attrs: { name: "importance" }  # Not yet part of the html spec
+  attrs: { name: "ismap" }
   attrs: { name: "intrinsicsize" }  # Not yet part of the html spec
   attrs: { name: "loading" }  # Not yet part of the html spec
   attrs: {
-    name: "src"
-    alternative_names: "srcset"
-    mandatory: true
+    name: "longdesc"
     value_url: {
-      protocol: "data"
-      protocol: "http" # For parity with amp-img's mandatory-src-or-srcset.
+      protocol: "http"
       protocol: "https"
-      allow_relative: true
     }
     disallowed_value_regex: "__amp_source_origin"
   }
   attrs: { name: "sizes" }
   attrs: { name: "width" }  # Not valid html5 but commonly used and supported.
+  attr_lists: "mandatory-src-or-srcset"
+  spec_url: "https://amp.dev/documentation/components/amp-img/"
 }
 # 4.7.2 The iframe element
 tags: {
@@ -4064,7 +4055,7 @@ tags: {
 # 4.11.3 The template element
 # See extensions/amp-mustache/validator-amp-mustache.protoascii.
 
-# 11 Obsolute features
+# 11 Obsolete features
 # 11.2 Non-conforming features
 tags: {
   html_format: AMP
@@ -4433,7 +4424,6 @@ tags: {
     mandatory: true
   }
 }
-
 tags: {
   html_format: AMP
   enabled_by: "transformed"
@@ -4463,6 +4453,36 @@ tags: {
     value: "async"
     mandatory: true
   }
+  attrs: {
+    name: "loading"
+    value: "lazy"
+    value: "eager"
+  }
+}
+tags: {
+  html_format: AMP
+  enabled_by: "transformed"
+  tag_name: "IMG"
+  spec_name: "amp-img > img[placeholder] (transformed)"
+  mandatory_parent: "amp-img (transformed)"
+  attrs: { name: "alt" }
+  attrs: { name: "attribution" }
+  attrs: {
+    name: "class"
+    mandatory: true
+    value: "i-amphtml-blurry-placeholder"
+  }
+  attrs: { name: "object-fit" }
+  attrs: { name: "object-position" }
+  attrs: {
+    name: "placeholder"
+    mandatory: true
+    value: ""
+  }
+  attrs: { name: "referrerpolicy" }
+  attrs: { name: "sizes" }
+  attrs: { name: "title" }
+  attr_lists: "mandatory-src-or-srcset"
 }
 
 # AMP Layout attributes: implied for TagSpecs with amp_layout field

@westonruter
Copy link
Member Author

@swissspidy do you need any of the story-related spec changes any time soon?

@westonruter westonruter marked this pull request as ready for review November 18, 2020 07:29
@github-actions
Copy link
Contributor

github-actions bot commented Nov 18, 2020

Plugin builds for c506569 are ready 🛎️!

@swissspidy
Copy link
Collaborator

@swissspidy do you need any of the story-related spec changes any time soon?

I don't think so! Thanks for asking 👍

@westonruter westonruter force-pushed the update/amphtml-2011140244000 branch from 4839a5f to 8425493 Compare November 20, 2020 00:00
@westonruter
Copy link
Member Author

@pierlon I'm not sure why c506569 is needed now, but it handles the case when WithoutBlockPreRendering hasn't created a $post, which I guess is why it was failing?

@westonruter westonruter requested a review from pierlon November 20, 2020 05:27
@westonruter westonruter added this to the v2.1 milestone Nov 20, 2020
@westonruter westonruter added the WS:Core Work stream for Plugin core label Nov 20, 2020
@@ -39,7 +39,7 @@ fi
if [[ -n $INSTALL_PWA_PLUGIN ]]; then
echo -n "Installing PWA plugin..."
wget -O "$WP_CORE_DIR/src/wp-content/plugins/pwa.zip" https://downloads.wordpress.org/plugin/pwa.zip
unzip -d "$WP_CORE_DIR/src/wp-content/plugins/pwa/" "$WP_CORE_DIR/src/wp-content/plugins/pwa.zip"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you come across this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a series of skipped tests in the PHPUnit output, and traced it back to the Test_AMP_Service_Worker class being skipped. Those tests are only skipped when the PWA plugin is not active.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PWA plugin was being extracted to the directory $WP_CORE_DIR/src/wp-content/plugins/pwa/pwa, which didn't match the hardcoded path used to activate the plugin.

@pierlon
Copy link
Contributor

pierlon commented Nov 20, 2020

@pierlon I'm not sure why c506569 is needed now, but it handles the case when WithoutBlockPreRendering hasn't created a $post, which I guess is why it was failing?

I suppose so, but why would it be failing now, and only on the PHP 8 job? I'm not able to replicate it with PHP 8 locally either.

@westonruter
Copy link
Member Author

Yeah, I was not able to reproduce it locally either. Nevertheless, it is better that the test has a global $post so that we can test for that context being added to the source stack.

@pierlon
Copy link
Contributor

pierlon commented Nov 20, 2020

Yea that's a plus.

@pierlon
Copy link
Contributor

pierlon commented Nov 20, 2020

Would we need to wait on a spec release that includes ampproject/amphtml#31241?

@westonruter
Copy link
Member Author

We can wait, but I'll do an update again anyway before we do 2.1, so it's not necessary.

Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes match changelog 👍

@westonruter westonruter merged commit a6bb693 into develop Nov 20, 2020
@westonruter westonruter deleted the update/amphtml-2011140244000 branch November 20, 2020 06:16
@westonruter westonruter modified the milestones: v2.1, v2.0.9 Dec 16, 2020
@westonruter westonruter modified the milestones: v2.0.9, v2.1 Dec 16, 2020
@westonruter westonruter mentioned this pull request Dec 16, 2020
6 tasks
@westonruter westonruter modified the milestones: v2.1, v2.0.10 Jan 13, 2021
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants